Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(node): never use relative import for node packages #352

Closed
wants to merge 2 commits into from
Closed

fix(node): never use relative import for node packages #352

wants to merge 2 commits into from

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Nov 29, 2024 •
edited
Loading

We should never import Nodejs packages with relative import as unenv will not get a chance to replace those.

Relates to opennextjs/opennextjs-cloudflare#147: It looks like the issue is caused by the unenv implementation of Readable#[Symbol.AsyncIterator]:

  async *[Symbol.asyncIterator](): NodeJS.AsyncIterator<any> {
    throw createNotImplementedError("Readable.asyncIterator");
  }

However this should never be used by workerd that has builtin stream.

The second commits add a dummy IncomingMessage#_read to make testing easier.

@vicb vicb requested review from a team and pi0 as code owners November 29, 2024 13:37
@pi0
Copy link
Member

pi0 commented Nov 29, 2024 •
edited
Loading

This breaks the direct fetch / http call mechanism of nitro, h3, and unenv itself in the Node.js environment (see this for ref), the reason is that Node.js polyfills of node:http in unenv depend on other node: polyfills (such as node:net) which are compatible with each other changed in this PR.

I think we need to find a different way to solve worked situation (simplest way I would imagine is to add any missing API to polyfills) and if you can kindly please open an issue with reproduction first I would be happy to help but so sorry this is really not a possible solution.

@pi0 pi0 closed this Nov 29, 2024
@pi0
Copy link
Member

pi0 commented Nov 29, 2024

(BTW generally we migrated and should change all other imports to node:, I noticed few node:events were relative we should fix only http/net/streams are that currently are functional with each other)

@vicb vicb mentioned this pull request Nov 29, 2024
Closed
@vicb vicb changed the title node-import fix(node): never use relative import for node packages Nov 29, 2024
@pi0 pi0 mentioned this pull request Nov 29, 2024
Closed
@vicb vicb mentioned this pull request Dec 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pi0 pi0 Awaiting requested review from pi0 pi0 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vicb @pi0